❮

Freeing sortgroupatts in use_physical_tlist

  • Jump to comment-1
    zyu@yugabyte.com2022-07-16T02:49:23+00:00
    Hi, I was looking at the code in use_physical_tlist(). In the code block checking CP_LABEL_TLIST, I noticed that the Bitmapset sortgroupatts is not freed before returning from the method. Looking at create_foreignscan_plan() (in the same file): bms_free(attrs_used); It seems the intermediate Bitmapset is freed before returning. I would appreciate review comments for the proposed patch. Thanks
    • Jump to comment-1
      tgl@sss.pgh.pa.us2022-07-16T03:32:54+00:00
      Zhihong Yu <zyu@yugabyte.com> writes: > I was looking at the code in use_physical_tlist(). > In the code block checking CP_LABEL_TLIST, I noticed that > the Bitmapset sortgroupatts is not freed before returning from the method. > Looking at create_foreignscan_plan() (in the same file): > bms_free(attrs_used); > It seems the intermediate Bitmapset is freed before returning. TBH, I'd say that it's probably the former code not the latter that is good practice. Retail pfree's in code that's not in a loop very possibly expend more cycles than they are worth, because the space will get cleaned up anyway when the active memory context is reset, and pfree is not as cheap as one might wish. It might be possible to make a case for one method over the other with some study of the particular situation, but you can't say a priori which way is better. On the whole, I would not bother changing either of these bits of code without some clear evidence that it matters. It likely doesn't. It's even more likely that it doesn't matter enough to be worth investigating. regards, tom lane
      • Jump to comment-1
        zyu@yugabyte.com2022-07-16T03:52:37+00:00
        On Fri, Jul 15, 2022 at 8:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Zhihong Yu <zyu@yugabyte.com> writes: > > I was looking at the code in use_physical_tlist(). > > In the code block checking CP_LABEL_TLIST, I noticed that > > the Bitmapset sortgroupatts is not freed before returning from the > method. > > Looking at create_foreignscan_plan() (in the same file): > > bms_free(attrs_used); > > It seems the intermediate Bitmapset is freed before returning. > > TBH, I'd say that it's probably the former code not the latter > that is good practice. Retail pfree's in code that's not in a > loop very possibly expend more cycles than they are worth, because > the space will get cleaned up anyway when the active memory > context is reset, and pfree is not as cheap as one might wish. > It might be possible to make a case for one method over the other > with some study of the particular situation, but you can't say > a priori which way is better. > > On the whole, I would not bother changing either of these bits > of code without some clear evidence that it matters. It likely > doesn't. It's even more likely that it doesn't matter enough > to be worth investigating. > > regards, tom lane > Hi, Tom: Thanks for responding over the weekend. I will try to remember what you said. Cheers